Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update Kotlin to version 2.1.0 #44809

Closed
wants to merge 2 commits into from
Closed

Update Kotlin to version 2.1.0 #44809

wants to merge 2 commits into from

Conversation

andreas-eberle
Copy link
Contributor

@andreas-eberle andreas-eberle commented Nov 28, 2024 •
edited
Loading

This PR updates Kotlin to the newly released version 2.1.0

@quarkus-bot quarkus-bot
Copy link

quarkus-bot bot commented Nov 28, 2024

Thanks for your pull request!

Your pull request does not follow our editorial rules. Could you have a look?

  • description should not be empty, describe your intent or provide links to the issues this PR is fixing (using Fixes #NNNNN) or changelogs

This message is automatically generated by a bot.

@quarkus-bot quarkus-bot bot added area/arc Issue related to ARC (dependency injection) area/dependencies Pull requests that update a dependency file area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/gradle Gradle labels Nov 28, 2024
@gsmet
Copy link
Member

gsmet commented Nov 29, 2024

Thanks for taking care of this!

@geoand
Copy link
Contributor

geoand commented Dec 3, 2024

Can you please rebase onto main?

@andreas-eberle andreas-eberle force-pushed the feature/kotlin-2.1.0 branch 2 times, most recently from c578c94 to 6404e78 Compare December 3, 2024 12:58
@andreas-eberle andreas-eberle marked this pull request as ready for review December 3, 2024 12:59
@andreas-eberle
Copy link
Contributor Author

Thanks for the reminder. I forgot I didn't push my last commit and undraft this. It should be updated and hopefully the CI passes now.

@geoand
Copy link
Contributor

geoand commented Dec 3, 2024

Thanks!

@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 3, 2024 •
edited
Loading

🎊 PR Preview 0ac0e12 has been successfully built and deployed to https://quarkus-pr-main-44809-preview.surge.sh/version/main/guides/

  • Images of blog posts older than 3 months are not available.
  • Newsletters older than 3 months are not available.

@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
@geoand
Copy link
Contributor

geoand commented Dec 3, 2024

CI didn't really run on this which is really weird

gsmet
gsmet requested changes Dec 3, 2024
View reviewed changes
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this effort, greatly appreciated.

I have a question though and I would like to get it sorted out before we merge. See inline.

...dard/src/main/kotlin/io/quarkus/it/resteasy/reactive/kotlin/CountryNamePayloadTransformer.kt
@@ -12,6 +12,6 @@ class CountryNamePayloadTransformer {
@Outgoing("countries-t1-out")
suspend fun transform(country: Country): Country {
delay(100)
return Country(country.name.toUpperCase(), country.capital)
return Country(country.name.lowercase(), country.capital)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it expected that you changed toUpperCase() to lowercase()? It might make sense but it looks odd to me?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is actually required. The toLowerCase method has been deprecated with a warning for a while but now they removed it or changed it to an error (I don't remember exactly any more, but the project didn't compile any longer). So this is a needed fix and lowercase() is the intended replacement.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expect uppercase() to be the replacement for toUpperCase() :).

@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
@geoand
Copy link
Contributor

geoand commented Dec 4, 2024 •
edited
Loading

Seems like quarkus-integration-test-kotlin is failing because of:

2024-12-04T07:18:19.3974147Z Caused by: io.quarkus.builder.BuildException: Build failure: Build failed due to errors
2024-12-04T07:18:19.3977020Z 	[error]: Build step io.quarkus.smallrye.graphql.deployment.SmallRyeGraphQLProcessor#buildExecutionService threw an exception: java.lang.IllegalArgumentException: Provided Metadata instance has version 2.1.0, while maximum supported version is 2.0.0. To support newer versions, update the kotlinx-metadata-jvm library.
2024-12-04T07:18:19.3979808Z 	at kotlinx.metadata.jvm.KotlinClassMetadata$Companion.throwIfNotCompatible$kotlinx_metadata_jvm(KotlinClassMetadata.kt:609)
2024-12-04T07:18:19.3981342Z 	at kotlinx.metadata.jvm.KotlinClassMetadata$Companion.checkMetadataVersionForRead(KotlinClassMetadata.kt:600)
2024-12-04T07:18:19.3982604Z 	at kotlinx.metadata.jvm.KotlinClassMetadata$Companion.read(KotlinClassMetadata.kt:579)
2024-12-04T07:18:19.3983607Z 	at kotlinx.metadata.jvm.KotlinClassMetadata.read(KotlinClassMetadata.kt)
2024-12-04T07:18:19.3984939Z 	at io.smallrye.graphql.schema.creator.OperationCreator.toKotlinClassMetadata(OperationCreator.java:264)
2024-12-04T07:18:19.3986391Z 	at io.smallrye.graphql.schema.creator.OperationCreator.checkWrappedTypeKotlinNullability(OperationCreator.java:126)
2024-12-04T07:18:19.3987866Z 	at io.smallrye.graphql.schema.creator.OperationCreator.createOperation(OperationCreator.java:112)
2024-12-04T07:18:19.3989154Z 	at io.smallrye.graphql.schema.SchemaBuilder.addOperations(SchemaBuilder.java:448)
2024-12-04T07:18:19.3990221Z 	at io.smallrye.graphql.schema.SchemaBuilder.lambda$generateSchema$2(SchemaBuilder.java:159)
2024-12-04T07:18:19.3991175Z 	at java.base/java.util.Optional.ifPresentOrElse(Optional.java:198)
2024-12-04T07:18:19.3992078Z 	at io.smallrye.graphql.schema.SchemaBuilder.generateSchema(SchemaBuilder.java:157)
2024-12-04T07:18:19.3993044Z 	at io.smallrye.graphql.schema.SchemaBuilder.build(SchemaBuilder.java:109)
2024-12-04T07:18:19.3994359Z 	at io.quarkus.smallrye.graphql.deployment.SmallRyeGraphQLProcessor.buildExecutionService(SmallRyeGraphQLProcessor.java:363)
2024-12-04T07:18:19.3995779Z 	at java.base/java.lang.invoke.MethodHandle.invokeWithArguments(MethodHandle.java:732)
2024-12-04T07:18:19.3996623Z 	at io.quarkus.deployment.ExtensionLoader$3.execute(ExtensionLoader.java:856)
2024-12-04T07:18:19.3997400Z 	at io.quarkus.builder.BuildContext.run(BuildContext.java:256)
2024-12-04T07:18:19.3998160Z 	at org.jboss.threads.ContextHandler$1.runWith(ContextHandler.java:18)
2024-12-04T07:18:19.3999290Z 	at org.jboss.threads.EnhancedQueueExecutor$Task.doRunWith(EnhancedQueueExecutor.java:2675)
2024-12-04T07:18:19.4000364Z 	at org.jboss.threads.EnhancedQueueExecutor$Task.run(EnhancedQueueExecutor.java:2654)
2024-12-04T07:18:19.4001695Z 	at org.jboss.threads.EnhancedQueueExecutor.runThreadBody(EnhancedQueueExecutor.java:1627)
2024-12-04T07:18:19.4002858Z 	at org.jboss.threads.EnhancedQueueExecutor$ThreadBody.run(EnhancedQueueExecutor.java:1594)
2024-12-04T07:18:19.4003716Z 	at java.base/java.lang.Thread.run(Thread.java:840)
2024-12-04T07:18:19.4004321Z 	at org.jboss.threads.JBossThread.run(JBossThread.java:499)

cc @jmartisk

@jmartisk
Copy link
Contributor

jmartisk commented Dec 4, 2024

Right, I'll try to upgrade kotlinx.metadata.jvm in smallrye-graphql, so this will have to wait until a smallrye-graphql upgrade is merged..

@geoand
Copy link
Contributor

geoand commented Dec 4, 2024

Thanks!

@jmartisk jmartisk mentioned this pull request Dec 4, 2024
Merged
@andreas-eberle
Copy link
Contributor Author

Thanks! Sorry I wasn't able too look for the errors in time. I'm unfortunately a bit swamped right now.

@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
@gsmet
Copy link
Member

gsmet commented Dec 5, 2024

Waiting for smallrye/smallrye-graphql#2234 to land in Quarkus.

@gsmet gsmet marked this pull request as draft December 5, 2024 15:42
@gsmet
Copy link
Member

gsmet commented Dec 20, 2024

@andreas-eberle probably a good idea to include these two changes in this PR:

A new version of SmallRye GraphQL was released so hopefully we will be able to make progress on this one soon!

@andreas-eberle
Copy link
Contributor Author

@gsmet: I've quickly updated the coroutines library and also the smallrye graphql dependency. Let's see if that already fixes the issues.

@gsmet
Copy link
Member

gsmet commented Dec 20, 2024

Yeah so the GraphQL one won't work as the update breaks things. We will have to wait for the SmallRye GraphQL team to push a proper update.

@andreas-eberle
Copy link
Contributor Author

Thanks for the info @gsmet. Then I'll rebase the PR as soon as the graphql update is properly integrated.

@jmartisk jmartisk mentioned this pull request Dec 20, 2024
Merged
@jmartisk
Copy link
Contributor

jmartisk commented Dec 20, 2024 •
edited
Loading

@andreas-eberle I've submitted #45228 that also incorporates your commit (and the coroutines upgrades mentioned earlier), I think we need to apply both upgrades at the same time. Please check it

@andreas-eberle
Copy link
Contributor Author

Thanks @jmartisk. Looks good for me from the kotlin update side. Let's see what CI says about your PR. I will close this one.

@geoand
Copy link
Contributor

geoand commented Dec 20, 2024

Closing in favor of #45228

@quarkus-bot quarkus-bot bot added the triage/invalid This doesn't seem right label Dec 20, 2024
@andreas-eberle andreas-eberle deleted the feature/kotlin-2.1.0 branch December 20, 2024 10:58
@andreas-eberle andreas-eberle mentioned this pull request Jan 9, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gsmet gsmet gsmet requested changes

@geoand geoand geoand approved these changes

Assignees
No one assigned
Labels
area/arc Issue related to ARC (dependency injection) area/dependencies Pull requests that update a dependency file area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/gradle Gradle triage/flaky-test triage/invalid This doesn't seem right
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@andreas-eberle @gsmet @geoand @jmartisk